[SPARK-3469] Make sure all TaskCompletionListener are called even with failures by rxin · Pull Request #2343 · apache/spark

rxin · 2014-09-10T06:34:44Z

This is necessary because we rely on this callback interface to clean resources up. The old behavior would lead to resource leaks.

Note that this also changes the fault semantics of TaskCompletionListener. Previously failures in TaskCompletionListeners would result in the task being reported immediately. With this change, we report the exception at the end, and the reported exception is a TaskCompletionListenerException that contains all the exception messages.

Note that this also changes the fault semantics of TaskCompletionListener. Previously failures in TaskCompletionListeners would result in the task being reported as failed. With this change, tasks won't be reported as failed simply because the execution of TaskCompletionListener fails.

rxin · 2014-09-10T08:12:13Z

Jenkins, add to whitelist.

rxin · 2014-09-10T08:12:19Z

Jenkins, test this please.

SparkQA · 2014-09-11T02:06:52Z

QA tests have started for PR 2343 at commit 1cb444d.

This patch merges cleanly.

SparkQA · 2014-09-11T02:10:16Z

QA tests have finished for PR 2343 at commit 1cb444d.

This patch fails unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2014-09-11T02:16:36Z

QA tests have started for PR 2343 at commit 1cb444d.

This patch merges cleanly.

SparkQA · 2014-09-11T02:52:39Z

QA tests have started for PR 2343 at commit 29b6162.

This patch merges cleanly.

SparkQA · 2014-09-11T03:17:21Z

QA tests have finished for PR 2343 at commit 1cb444d.

This patch passes unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2014-09-11T03:56:54Z

QA tests have finished for PR 2343 at commit 29b6162.

This patch passes unit tests.
This patch merges cleanly.
This patch adds no public classes.

shivaram · 2014-09-11T07:22:44Z

Do we need to change the semantics though ? We could still save the first exception and return a task failure ?

rxin · 2014-09-12T07:45:46Z

What do you think would be a good way to handle this?

Perhaps throw back the first exception encountered? or throw an exception with all the error messages

rxin · 2014-09-12T07:56:56Z

Ok I pushed a new version that creates a TaskCompletionListenerException that contains all the error messages.

SparkQA · 2014-09-12T07:59:14Z

QA tests have started for PR 2343 at commit aa68ea4.

This patch merges cleanly.

SparkQA · 2014-09-12T08:51:14Z

QA tests have finished for PR 2343 at commit aa68ea4.

This patch fails unit tests.
This patch merges cleanly.
This patch adds no public classes.

shivaram · 2014-09-12T22:42:32Z

core/src/main/scala/org/apache/spark/TaskContext.scala

update this comment

shivaram · 2014-09-12T22:44:23Z

I left a minor comment - Otherwise LGTM

shivaram · 2014-09-12T22:44:59Z

Jenkins, retest this please

SparkQA · 2014-09-13T00:44:25Z

QA tests have started for PR 2343 at commit ac5baea.

This patch merges cleanly.

SparkQA · 2014-09-13T01:48:46Z

QA tests have finished for PR 2343 at commit ac5baea.

This patch passes unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- throw new IllegalStateException("The main method in the given main class must be static")
- class TaskCompletionListenerException(errorMessages: Seq[String]) extends Exception

rxin · 2014-09-13T04:55:28Z

Ok merging this in master. Thanks for taking a look.

oops compilation failed.

29b6162

rxin closed this Sep 12, 2014

rxin reopened this Sep 12, 2014

Throw an exception if task completion callback fails.

aa68ea4

rxin changed the title ~~[SPARK-3469] Call all TaskCompletionListeners even if some fail~~ [SPARK-3469] Make sure all TaskCompletionListener are called even with failures Sep 12, 2014

shivaram reviewed Sep 12, 2014
View reviewed changes

core/src/main/scala/org/apache/spark/TaskContext.scala Outdated

Copy link

Contributor

shivaram Sep 12, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update this comment

Removed obsolete comment.

ac5baea

Mark TaskCompletionListenerException as private[spark].

a3845b2

asfgit closed this in 2584ea5 Sep 13, 2014

Conversation

rxin commented Sep 10, 2014

Uh oh!

rxin commented Sep 10, 2014

Uh oh!

rxin commented Sep 10, 2014

Uh oh!

SparkQA commented Sep 11, 2014

Uh oh!

SparkQA commented Sep 11, 2014

Uh oh!

SparkQA commented Sep 11, 2014

Uh oh!

SparkQA commented Sep 11, 2014

Uh oh!

SparkQA commented Sep 11, 2014

Uh oh!

SparkQA commented Sep 11, 2014

Uh oh!

shivaram commented Sep 11, 2014

Uh oh!

rxin commented Sep 12, 2014

Uh oh!

rxin commented Sep 12, 2014

Uh oh!

SparkQA commented Sep 12, 2014

Uh oh!

SparkQA commented Sep 12, 2014

Uh oh!

shivaram Sep 12, 2014

Choose a reason for hiding this comment

Uh oh!

shivaram commented Sep 12, 2014

Uh oh!

shivaram commented Sep 12, 2014

Uh oh!

SparkQA commented Sep 13, 2014

Uh oh!

SparkQA commented Sep 13, 2014

Uh oh!

rxin commented Sep 13, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants